[WIP] *: support layers federation #147

runcom · 2016-11-02T09:21:27Z

More information available at moby/moby#27961

Signed-off-by: Antonio Murdaca [email protected]

runcom · 2016-11-02T12:15:03Z

@mtrmac I just like to understand how you feel about this, I know the code is broken and need a look of fixing around "UpdatedImage" and such

mtrmac

Overall, well, if it is a needed feature supported by docker/distribution, we will need to support it, so what’s not to like?

I would, however, like to see a comprehensive documentation somewhere of what this is supposed to do and why and how. Right now this adds URLs without any documentation; is the semantics that these layers should not or must not be copied? Will they always be accessible without authentication? (Perhaps these things are already documented somewhere in docker/distribution?)

And it might be desirable to have tests for schema conversion before this lands, and tests for how schema conversion interacts with this, and perhaps also tests for interaction with containers/storage. This kind of change seems somewhat likely to break something without us noticing.

mtrmac · 2016-11-02T15:28:24Z

copy/copy.go

+				}
+				cl = copiedLayer{blobInfo: destInfo, diffID: diffID}
+			} else {
+				fmt.Fprintf(reportWriter, "Skipping foreign layer copy to %s\n", dest.Reference().Transport().Name())


We still need to fill in info for manifestUpdates.{InformationOnly.,}LayerInfos and possibly for manifestUpdates.InformationOnly.LayerDiffIDs which is much more horrible.

Also we might want to record URLs for the newly created layers as well.

mtrmac · 2016-11-02T15:30:42Z

copy/copy.go

+		err  error
+	)
+	for _, url := range srcInfo.URLs {
+		resp, err = http.Get(url)


Implementing a specific transport like this in the generic copy code is weird. Why wouldn’t this be done by an ImageSource, perhaps through ImageSource.GetBlob(info types.BlobInfo)?

In particular, does the access never need any authentication?

In particular, does the access never need any authentication?

answer to this is, sadly, we don't yet know, ongoing discussion at moby/moby#27961

Implementing a specific transport like this in the generic copy code is weird. Why wouldn’t this be done by an ImageSource, perhaps through ImageSource.GetBlob(info types.BlobInfo)?

makes sense

mtrmac · 2016-11-02T15:31:50Z

oci/layout/oci_dest.go

+// uploaded to the image destination, false otherwise.
+func (d *ociImageDestination) CopyForeignLayers() bool {
+	return false
+}


Why is this true for dir: but false for oci:? At a first glance it seems that the two should behave similarly.

mtrmac · 2016-11-02T15:32:48Z

types/types.go


+	// CopyForeignLayers returns true iff foreign layers in manifest should be actually
+	// uploaded to the image destination, false otherwise.
+	CopyForeignLayers() bool


Perhaps ShouldCopyForeignLayers or SupportsForeignLayers or something; this reads like a command to copy them.

runcom · 2016-11-15T10:03:11Z

I would, however, like to see a comprehensive documentation somewhere of what this is supposed to do and why and how. Right now this adds URLs without any documentation; is the semantics that these layers should not or must not be copied? Will they always be accessible without authentication? (Perhaps these things are already documented somewhere in docker/distribution?)

moby/moby#27961

@mtrmac PTAL (skopeo PR containers/skopeo#249)

runcom · 2016-11-16T21:53:18Z

docker/docker_image_src.go

+		err  error
+	)
+	for _, url := range urls {
+		resp, err = http.Get(url)


we should use the previously setup docker client to comply with containers/skopeo#250

runcom · 2016-11-23T16:51:03Z

ping @mtrmac anything else?

runcom · 2016-11-25T10:26:49Z

rebased

mtrmac

The highlights are authentication and the naming vs. semantics of SupportsForeignLayers.

Also, shouldn’t convertToManifestSchema1 refuse to convert images with federated URLs?

mtrmac · 2016-11-25T19:02:26Z

image/docker_schema2.go

 			return nil, fmt.Errorf("Internal error: neither src nor configBlob set in manifestSchema2")
 		}
-		stream, _, err := m.src.GetBlob(m.ConfigDescriptor.Digest)
+		stream, _, err := m.src.GetBlob(types.BlobInfo{Digest: m.ConfigDescriptor.Digest})


Shouldn’t this fully convert the descriptor, including URLs?

At the very least, Size should be -1, not 0.

mtrmac · 2016-11-25T19:03:42Z

types/types.go

 	GetTargetManifest(digest string) ([]byte, string, error)
 	// GetBlob returns a stream for the specified blob, and the blob’s size (or -1 if unknown).
-	GetBlob(digest string) (io.ReadCloser, int64, error)
+	GetBlob(BlobInfo) (io.ReadCloser, int64, error)


Is the caller expected to fill in everything, in particular is Size required?

(AFAICT we don’t need it, so this would be just a matter of documenting The Digest field is guaranteed to be provided; Size may be -1 just as we do for LayerInfos where this information comes from.)

mtrmac · 2016-11-25T19:07:32Z

docker/docker_image_src.go

+	return nil, 0, err
+}
+
+func getBlobSizeFromHeader(h string) int64 {


This could get a http.Response and look up the Content-Length header itself. This works as well, but the FromHeader naming is a bit confusing, when this is just a decimal parser.

(Also there is another parser of Content-Length in docker_image_dest.go which could be modified to call this.)

mtrmac · 2016-11-25T19:09:22Z

docker/docker_image_src.go

+		err  error
+	)
+	for _, url := range urls {
+		resp, err = s.c.makeRequestToResolvedURL("GET", url, nil, nil, -1)


I know I asked about authentication; still, is this right?

This would, when pulling a foreign RHEL from docker.io, send the docker.io credentials to Red Hat.

Right, we just need the client w/o auth here because we may need auth certificates against the give foreign server. We may probably need a new method in the client to make calls w/o authentication but still honor cert-path and the like for the given server we're connecting to here.

Yeah, using the certs but not sending any tokens/passwords is AFAICT safe.

Ultimately, we would need to decide where the tokens/passwords are configured, and whether that is a per-transport matter or something general, and so on, but if we don’t have any server needing this right now, let's get this merged first and worry about that later.

mtrmac · 2016-11-25T19:11:26Z

copy/copy.go

 func copyLayer(dest types.ImageDestination, src types.ImageSource, srcInfo types.BlobInfo,
 	diffIDIsNeeded bool, canCompress bool, reportWriter io.Writer) (types.BlobInfo, string, error) {
-	srcStream, srcBlobSize, err := src.GetBlob(srcInfo.Digest) // We currently completely ignore srcInfo.Size throughout.
+	srcStream, srcBlobSize, err := src.GetBlob(srcInfo) // We currently completely ignore srcInfo.Size throughout.


Is the srcInfo.Size comment still accurate? (See also types.go)

mtrmac · 2016-11-25T19:17:19Z

copy/copy.go

+			)
+			if !dest.SupportsForeignLayers() && len(srcLayer.URLs) != 0 {
+				destInfo = srcLayer
+				// TODO: diffID?


As it happens, DiffIDs are, at least currently, needed only when converting from schema1, in which case src.LayerInfos will not have URLs because schema1 does not support them. So, luckily enough, we don’t need to be handling this; just add a check

if diffIDsAreNeeded { return errors.New("getting DiffIDs for foreign layers is unimplemented") // and a comment explaining schema1 }

mtrmac · 2016-11-25T19:18:15Z

copy/copy.go

+			if !dest.SupportsForeignLayers() && len(srcLayer.URLs) != 0 {
+				destInfo = srcLayer
+				// TODO: diffID?
+				fmt.Fprintf(reportWriter, "Skipping foreign layer copy to %s\n", dest.Reference().Transport().Name())


This should probably identify the blob by digest (and the Copying Blob message above should go into the else branch below).

mtrmac · 2016-11-25T19:23:10Z

types/types.go


+	// SupportsForeignLayers returns true iff foreign layers in manifest should be actually
+	// uploaded to the image destination, false otherwise.
+	SupportsForeignLayers() bool


This naming is pretty confusing; this should return false exactly in the cases when the destination understands URLs.

As is, this is really ShouldConvertForeignLayersToBlobCopies :) Or perhaps keep the current semantics and call it AcceptsForeignLayerURLs?

mtrmac · 2016-11-25T19:25:23Z

oci/layout/oci_dest.go

+			if len(l.URLs) != 0 {
+				om.Layers[i].MediaType = imgspecv1.MediaTypeImageLayerNonDistributable
+			} else {
+				om.Layers[i].MediaType = imgspecv1.MediaTypeImageLayer


If I remember correctly the OCI discussion of URLs vs. MIME types, URLs means “pull from here” and a MIME type can be used to indicate “don’t push this”; if so, the OCI MIME type should be based on whether the Docker MIME type is a …rootfs.foreign.diff…, not on the presence of URLs.

Or is the semantics of URLs vs. MIME type different between Docker s2 and OCI?

runcom · 2016-11-26T10:46:15Z

@mtrmac updated and fixed your comments. PTAL

runcom · 2016-11-28T16:18:07Z

rebased again.

mtrmac · 2016-11-29T20:39:13Z

types/types.go


+	// AcceptsForeignLayerURLs returns true iff foreign layers in manifest should be actually
+	// uploaded to the image destination, false otherwise.
+	AcceptsForeignLayerURLs() bool


I’m sorry, I have suggested this exact wording and this is entirely my fault, but AcceptsForeignLayerURLs is the opposite of what the flag means: false = keep the BlobInfo with the URL, true = make a copy.

(Writing this out in case I am making another mistake.)

Should I rework the comment here and swap true with false then right?

runcom · 2016-11-30T16:33:25Z

@mtrmac updated and rebased

mtrmac · 2016-11-30T16:56:46Z

I still see AcceptsForeignLayerURLs false for docker:/atomic:, and false causing the URLs to be preserved, i.e. no change ?!

runcom · 2016-11-30T17:02:37Z

I still see AcceptsForeignLayerURLs false for docker:/atomic:, and false causing the URLs to be preserved, i.e. no change ?!

How about now? I swapped all occurrences I guess

mtrmac · 2016-11-30T17:14:43Z

Missing the use in copy.go AFAICS

runcom · 2016-11-30T17:16:17Z

Missing the use in copy.go AFAICS

yep, sorry, forget me, I updated the use in copy as well.

mtrmac · 2016-11-30T17:19:29Z

👍

Thanks!

runcom · 2016-11-30T17:22:44Z

lgtm

thanks @mtrmac sorry for the back and forth

runcom · 2016-11-30T17:23:02Z

I'll fix the skopeo PR before merging here, stay tuned

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2016-11-30T18:47:14Z

containers/skopeo#249 is good to go as well, merging here

Add a CONTRIBUTING.md file

runcom force-pushed the layers-federation branch from a387cea to f27ff17 Compare November 2, 2016 10:03

runcom changed the title *: support layers federation [WIP] *: support layers federation Nov 2, 2016

mtrmac reviewed Nov 2, 2016

View reviewed changes

runcom force-pushed the layers-federation branch 5 times, most recently from 1f99fdc to 1bcf590 Compare November 15, 2016 10:01

runcom mentioned this pull request Nov 15, 2016

Support layers federation containers/skopeo#249

Merged

runcom force-pushed the layers-federation branch 2 times, most recently from c42f15a to f7d5778 Compare November 15, 2016 10:59

mtrmac mentioned this pull request Nov 15, 2016

types.BlobInfo is basically a Descriptor #164

Closed

runcom commented Nov 16, 2016

View reviewed changes

runcom force-pushed the layers-federation branch 2 times, most recently from f4bb04e to 488f068 Compare November 23, 2016 16:50

runcom force-pushed the layers-federation branch from 488f068 to 537ea34 Compare November 25, 2016 10:26

mtrmac requested changes Nov 25, 2016

View reviewed changes

runcom force-pushed the layers-federation branch from 537ea34 to 87cb0b5 Compare November 26, 2016 10:45

runcom force-pushed the layers-federation branch from 87cb0b5 to 0dc9b44 Compare November 28, 2016 16:17

mtrmac reviewed Nov 29, 2016

View reviewed changes

runcom force-pushed the layers-federation branch from 0dc9b44 to b19dce0 Compare November 30, 2016 16:33

runcom force-pushed the layers-federation branch from b19dce0 to 60ff2bc Compare November 30, 2016 17:02

runcom force-pushed the layers-federation branch from 60ff2bc to d1103c5 Compare November 30, 2016 17:16

mtrmac approved these changes Nov 30, 2016

View reviewed changes

*: support layers federation

d54f115

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the layers-federation branch from d1103c5 to d54f115 Compare November 30, 2016 17:55

runcom merged commit 3040b28 into containers:master Nov 30, 2016

runcom deleted the layers-federation branch November 30, 2016 18:47

giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017

Merge pull request containers#147 from duglin/contrib

5197c8d

Add a CONTRIBUTING.md file

mtrmac mentioned this pull request Sep 28, 2017

Support to download remote OCI layers (with TLS) #351

Merged

mtrmac mentioned this pull request Apr 29, 2019

Support managing manifests with foreign URLs containers/skopeo#247

Closed

[WIP] *: support layers federation #147

[WIP] *: support layers federation #147

Uh oh!

Conversation

runcom commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Nov 2, 2016

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Nov 23, 2016

Uh oh!

runcom commented Nov 25, 2016

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

runcom commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Nov 30, 2016

Uh oh!

mtrmac commented Nov 30, 2016

Uh oh!

runcom commented Nov 2, 2016 •

edited

Loading

runcom commented Nov 15, 2016 •

edited

Loading

mtrmac Nov 25, 2016 •

edited

Loading

mtrmac commented Nov 30, 2016 •

edited by runcom

Loading

runcom commented Nov 30, 2016 •

edited

Loading